Add witness hints fillers #169
Conversation
Nashtare
left a comment
Nashtare
left a comment
There was a problem hiding this comment.
Thanks Alonso!
As a general comment, I feel like Unconstrained is a bit misleading, as we may still apply constraints on these values (like in hash_squeeze for instance), so perhaps it'd be worth thinking of another name, maybe something like Op::Private or anything that reflects the underlying logic?
| println!("=== CIRCUIT PRIMITIVE OPERATIONS ==="); | ||
| for (i, prim) in circuit.primitive_ops.iter().enumerate() { | ||
| println!("{i}: {prim:?}"); | ||
| } |
There was a problem hiding this comment.
Can we remove all those prints?
There was a problem hiding this comment.
These are the same as in the original test. I forgot to mention that I think the new test "Proves that we know x such that 37 * x - 111 = 0" (like a NP statement), as the comment says. In the original the solution is also know to the verifer, so it's more like "Proves that 3 is a solution to 37 * x - 111 = 0" (P statement). Should we keep both tests?
Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com>
In my opinion it's is indeed unconstrained, until you push the non-primitive operation (e.g. what should happen in |
| &self, | ||
| traces: &Traces<EF>, | ||
| pis: &Vec<Val<SC>>, | ||
| pis: &[Val<SC>], |
There was a problem hiding this comment.
This (and other changes) have nothing to do with the PR, but clippy was complaining
There was a problem hiding this comment.
yes it's been merged on the Plonky3 side
| bits.push(bit); | ||
| } | ||
| let binary_decomposition_hint = BinaryDecompositionHint::new(x, n_bits)?; | ||
| let bits = builder.alloc_witness_hints(binary_decomposition_hint, "decompose_to_bits"); |
There was a problem hiding this comment.
Should we add back builder.assert_bool(bit);?
| use alloc::{format, vec}; | ||
| use core::fmt::Debug; | ||
| use core::iter; | ||
| use core::result::Result; |
There was a problem hiding this comment.
Is this (still) used in the file?
| assert_eq!(traces.add_trace.lhs_values.len(), 1); | ||
| assert_eq!(traces.add_trace.lhs_index, vec![WitnessId(2)]); | ||
| assert_eq!(traces.add_trace.rhs_index, vec![WitnessId(0)]); | ||
| assert_eq!(traces.add_trace.result_index, vec![WitnessId(4)]); |
There was a problem hiding this comment.
Can we add an assert that the x value is 3?
| .map(|&index_target| { | ||
| let all_bits = decompose_to_bits(circuit, index_target, MAX_QUERY_INDEX_BITS); | ||
| let all_bits = | ||
| decompose_to_bits(circuit, index_target, MAX_QUERY_INDEX_BITS).unwrap(); |
There was a problem hiding this comment.
Should we map the CircuitError to a VerificationError instead of using unwrap() here?
|
|
||
| // Decompose to bits (adds public inputs for each bit and verifies they reconstruct x) | ||
| let bits = decompose_to_bits(circuit, x, total_num_bits); | ||
| let bits = decompose_to_bits(circuit, x, total_num_bits).unwrap(); |
There was a problem hiding this comment.
I think we should make the method return an Error instead of unwrapping
Currently, witness hints can be declared using
alloc_witness_hints, but there is no defined mechanism for how these values are populated during circuit evaluation. This makes it unclear how unconstrained data should be assigned within the witness table.This PR introduces a new primitive operation,
Op::Unconstrained, which assigns the values of witness hints. During evaluation time, these values are populated according to the behavior defined by the new WitnessHintFiller trait. Implementors of this trait define the logic for deriving hint values, potentially using existing wire values already computed in the circuit.